Make useStyleOverride public #63656
Merged
Make useStyleOverride public #63656
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
added
[Type] New API
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Pixelous. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -1 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I like the idea of making this public — it has a good and simply interface (optional id + CSS string), and I can imagine it being useful for custom block plugins as in the linked issue.
A general question: are there any performance concerns if lots of custom blocks wind up using this? I imagine not since it's already in use for the block supports. But just in case, I'll add a ping to @WordPress/gutenberg-core for visibility 🙂
Comment on lines
142
to
145
| * @param {Object} $0 Named parameters. | ||
| * @param {string} $0.id Id of the style override, leave blank to create a new | ||
| * style. | ||
| * @param {string} $0.css CSS to apply. |
There was a problem hiding this comment.
Tiny nit: should this be an optional string param?
Suggested change
| * @param {Object} $0 Named parameters. | |
| * @param {string} $0.id Id of the style override, leave blank to create a new | |
| * style. | |
| * @param {string} $0.css CSS to apply. | |
| * @param {Object} $0 Named parameters. | |
| * @param {?string} $0.id Id of the style override, leave blank to create a new | |
| * style. | |
| * @param {string} $0.css CSS to apply. |
There was a problem hiding this comment.
If we're flagging id as optional here, we should probably also then do the same in the readme.
There was a problem hiding this comment.
I think JSDocs also supports a better style than $0 for object params.
There was a problem hiding this comment.
@Mamaduka Curious what you're referring to. Do you mean a separate type definition?
There was a problem hiding this comment.
* @param {Object} override
* @param {string} override.id
* @param {string} override.css
| @@ -265,7 +265,7 @@ function useDuotoneStyles( { | |||
|
|
|||
| const isValidFilter = Array.isArray( colors ) || colors === 'unset'; | |||
|
|
|||
| useStyleOverride( | |||
| usePrivateStyleOverride( | |||
There was a problem hiding this comment.
Tiny nit about consistency:
I see Duotone and block style variations have been switched to use usePrivateStyleOverride instead of useStyleOverride. Is there a reason to switch these internal-to-the-block-editor instances to use the private one? If so, would it be worth updating the code in layout-child.js, layout.js, position.js and style.js, too? If not, then I was wondering if it would be better for these ones to use useStyleOverride instead of the private hook 🤔
Edit: ignore me, sorry! I only just noticed the __unstableType: 'presets' line here 🤦
I see now why a couple of these use the private version and the others continue to use the public one 👍
There was a problem hiding this comment.
All good, I typed before thinking that comment through! 😄
There was a problem hiding this comment.
I believe the block style variations and duotone use the private version as they rely on the restricted parameters we're deliberately omitting from the public useStyleOverride e.g. clientId, __unstableType etc.
There was a problem hiding this comment.
Thanks for confirming!
aaronrobertshaw
approved these changes
Jul 18, 2024
There was a problem hiding this comment.
I like the idea of making useStyleOverride public. Bonus points for the private version to restrict some of the accepted params ✨
✅ Adding custom useStyleOverride to block works
✅ Duotone and Block Style Variations work still using private version
✅ Gallery gap styles work
✅ General layout styles continue to work
Other than the docblock/readme nit, mentioned in other comments, this PR LGTM 👍
There was a problem hiding this comment.
I won't block this, but I don't think this is the right decision because it will make emotion an irrevocable part of WordPress forever. As @mirka said here, we're planning to move away from Emotion, as many are for important reasons like performance, bundle size, and incompatibility with modern React features. The MUI team wrote about this.
In the near term, there are options that address these same challenges that are much more aligned with the web platform and the future of the fundamental tools we rely on, such as React. Some examples: the new React 19 style tag features, CSS layers, :where...
So I would recommend not shipping this, at least not for now, until we have more clarity on the post-Emotion future in the components library and other places in Gutenberg.
cc @WordPress/gutenberg-components
|
@DaniGuardiola I might be missing something but I don't think making The style overrides added through this hook are stored in a registry and later retrieved and rendered within |
|
@aaronrobertshaw it's very likely that I'm confused if that's the case. I didn't look too deep into the code, so I only have the context of the original issue which was about exposing some emotion utilities. Happy to have been proven wrong! |
|
@DaniGuardiola This is the alternative to emotion as discussion on the issue :) |
ellatrix
force-pushed
the
expose/use-style-override
branch
from
df6ca27 to
8cea193
Compare
|
Flaky tests detected in 8cea193. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10169557719
|
fabiankaegy
added
the
Needs Dev Note
97 tasks
|
Hey @ellatrix 👋 Would you be able to help write a dev note for this for the 6.7 release? We are planning to have this as part of a larger Miscellaneous Editor Updates note. We are hoping to get all drafts in by October 13th to leave some time for reviews before the RC1. All Dev Notes get tracked in #65784 so feel free to leave a note there or ping me directly :) Please let us know if you can assist with that. Thanks in advance :) |
|
I don't have much writing inspiration, something as simple as: |
|
@ellatrix i think what I personally was hoping to have included here would be a very small example of how you would actually pass on styles etc. |
|
@fabiankaegy Sorry, there's one in the description: |
|
Perfect thanks :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #61232. Exposes
useStyleOverridesso styles can be added to the canvas by blocks.Why?
See #61232 (comment). It's handy to have an API for dynamically adding editor styles.
How?
Exposed the hooks while keeping private props internal for now. Only
idandcssare exposed.Testing Instructions
useStyleOverride( { css: 'p{color:red}' } )in a block. Note that most often this is used in combination with a generated class and css specifically targeting that class.Testing Instructions for Keyboard
Screenshots or screencast